ref: fix typing of jira_server.integration #86388
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #86388 +/- ##
==========================================
- Coverage 87.85% 87.84% -0.01%
==========================================
Files 9736 9736
Lines 552361 552354 -7
Branches 21539 21539
==========================================
- Hits 485258 485217 -41
- Misses 66717 66751 +34
Partials 386 386 |
| self._org_integration: RpcOrganizationIntegration | None | ||
|
|
||
| @property | ||
| def org_integration(self) -> RpcOrganizationIntegration | None: | ||
| @cached_property | ||
| def org_integration(self) -> RpcOrganizationIntegration: | ||
| from sentry.integrations.services.integration import integration_service | ||
|
|
||
| if not hasattr(self, "_org_integration"): | ||
| self._org_integration = integration_service.get_organization_integration( | ||
| integration_id=self.model.id, | ||
| organization_id=self.organization_id, | ||
| ) | ||
| return self._org_integration | ||
|
|
||
| @org_integration.setter | ||
| def org_integration(self, org_integration: RpcOrganizationIntegration) -> None: | ||
| self._org_integration = org_integration | ||
| integration = integration_service.get_organization_integration( | ||
| integration_id=self.model.id, | ||
| organization_id=self.organization_id, | ||
| ) | ||
| if integration is None: | ||
| raise NotFound("missing org_integration") | ||
| return integration |
There was a problem hiding this comment.
this is the "big" change in this changeset that I'm not certain about -- it makes it so that we can't construct an integration with a missing OrganizationIntegration
I'm not certain that this is correct -- but it seems most of the integrations do not handle the case where this is missing and as far as I can understand it doesn't really make sense to have an integration not associated with an organization -- will need the domain owners to confirm this idea though
There was a problem hiding this comment.
I brought this up with my team to double check, and we think this is a fair assumption to make. Most of our integrations operate with the assumption that we have an org integration already, so it should be fine to handle the potentially few cases that don't explicitly with a try/catch.
| integration_service.update_integration( | ||
| integration_id=self.model.id, | ||
| name=self.model.name, | ||
| metadata=self.model.metadata, | ||
| ) |
There was a problem hiding this comment.
Was this change made to address the case where model is an RpcIntegration?
There was a problem hiding this comment.
yep -- it would otherwise crash with AttributeError: save
| self._org_integration: RpcOrganizationIntegration | None | ||
|
|
||
| @property | ||
| def org_integration(self) -> RpcOrganizationIntegration | None: | ||
| @cached_property | ||
| def org_integration(self) -> RpcOrganizationIntegration: | ||
| from sentry.integrations.services.integration import integration_service | ||
|
|
||
| if not hasattr(self, "_org_integration"): | ||
| self._org_integration = integration_service.get_organization_integration( | ||
| integration_id=self.model.id, | ||
| organization_id=self.organization_id, | ||
| ) | ||
| return self._org_integration | ||
|
|
||
| @org_integration.setter | ||
| def org_integration(self, org_integration: RpcOrganizationIntegration) -> None: | ||
| self._org_integration = org_integration | ||
| integration = integration_service.get_organization_integration( | ||
| integration_id=self.model.id, | ||
| organization_id=self.organization_id, | ||
| ) | ||
| if integration is None: | ||
| raise NotFound("missing org_integration") | ||
| return integration |
There was a problem hiding this comment.
I brought this up with my team to double check, and we think this is a fair assumption to make. Most of our integrations operate with the assumption that we have an org integration already, so it should be fine to handle the potentially few cases that don't explicitly with a try/catch.
No description provided.